Repository navigation
fix(monitors): preserve price-token boundaries and normalized contains matching - #172
charan-rathore wants to merge 3 commits into
Conversation
matchesPrice consumed a numeric prefix of malformed amounts: $1,00 and USD1,00 became 1, and $9.999 became 9.99, so a threshold of 10 matched and sent a notification for a page that never showed a real price under it. Reject a match when the next character continues the numeric token (digit or comma, or a decimal dot followed by a digit); slash and sentence punctuation stay separators. Regression cases run the real browser-fixture, API and worker outcome path. Signed-off-by: Charan Rathore <180254320+charan-rathore@users.noreply.github.com>
NathanTarbert
left a comment
NathanTarbert
left a comment
There was a problem hiding this comment.
Thanks @charan-rathore. Stopping "$1,000" from being read as "$1" is a good catch, and the lookahead does that. One case needs adjusting, because it now misses real prices.
A price followed by a comma no longer matches. The new lookahead (?![\d,]|\.\d) rejects any comma after the number, including a normal comma in a sentence. So "Now $9, was $20" only finds $20, and "Only $5," finds nothing. A watch for prices under $10 would never fire on that page. Rejecting a comma only when a digit follows it, as in (?!\d|,\d|\.\d), keeps the fix for "$1,000" and lets ordinary commas through. A couple of test cases like "Price: $9, limited time" and "$5, $20" would lock that in.
A related gap: a price with a letter after it still gets cut short. "$9M" and "$9k" still read as 9, so a watch for prices under 10 would fire on something worth 9 million. Requiring that the number isn't followed by a letter would cover those along with the punctuation cases.
The tests could also work a bit harder. The "$1,000,00" and "$1,000.00" cases come out the same with or without the fix, since both read as 1000, which isn't under 10. So they don't actually test the new check. A higher threshold, or a case like "$1,0", would make them meaningful. The new file is also mostly a copy of tests/monitor-price-spacing.test.ts, including the owner id price-spacing-user, and each case starts a whole browser. matchesPrice only works on a string, so exporting it would allow quick table tests of all these inputs, with one end-to-end case kept for the wiring. That would probably also fix the slow runs mentioned in the description. Last, the test title says "does not truncate malformed numeric tokens" for every case, including the valid ones that should match, so a failure on "$9.99/month" would show up under a misleading name.
…price_below The malformed-token lookahead also rejected an ordinary comma after a real price, so "Only $5," or "Price: $9, limited time" never matched and a watch could not fire. A price with a magnitude letter still truncated: $9M and $9k read as 9. Reject only a digit, a comma followed by a digit, a decimal dot followed by a digit, or a trailing letter after the amount. Extract matchesPrice into apps/server/src/engine/price.ts and cover it with fast table cases (comma sentences, lists, magnitude suffixes, truncated groups); keep one browser-fixture case for the worker wiring. Signed-off-by: Charan Rathore <180254320+charan-rathore@users.noreply.github.com>
|
Thanks, both catches reproduce. The lookahead rejected an ordinary sentence comma, so "Only $5," and "Now $9, was $20" missed the real price, and "$9M" / "$9k" still truncated to 9. The lookahead now rejects only a digit, a comma followed by a digit, a decimal dot followed by a digit, or a trailing letter. I checked the full table against the old and new patterns: "$1,00", "$1,0", "$9.999", "USD 1,00" and "$1,000,00" stay rejected, "$9M"/"$9k" no longer match, and "$9.99/month" still does. Good call on the tests. matchesPrice is now exported from apps/server/src/engine/price.ts, so the cases run as fast table tests with names that say what each pins, including "Price: $9, limited time" and "$5, $20". One browser-fixture case stays for the worker wiring, under its own owner id. The "$1,000,00" and "$1,000.00" entries now state what they actually check, and "$1,0" covers the truncated-group path. 16/16 table and wiring cases pass, tsc --noEmit is clean, biome is clean. One honest caveat: the neighboring monitor-price-spacing file gets killed partway through on this machine with and without this change (a memory limit here, not an assertion failure); every subtest that ran passed. |
jerelvelarde
left a comment
jerelvelarde
left a comment
There was a problem hiding this comment.
Reviewed b125de370fdeaf92006319c0ef1800e8c2305e7f.
Useful price-token hardening; 24 focused tests passed, including every new malformed-token case and the existing spacing suite. The earlier comma and magnitude-suffix feedback is addressed by the current lookahead. Security: checked parser boundaries and the worker outcome path; no new external-write authority or URL behavior; no actionable security finding. Integration hold: GitHub reports CONFLICTING with current main after #185 normalized the contains-watch value. Rebase the production matcher branch while retaining normalized contains matching and replacing only the price matcher with the extracted helper. Run price-token, price-spacing and contains-spacing regressions together. Update the stale test-count/partial-run notes in the description. Keep open; this is complementary to #16 rather than a duplicate.
Local execution used read-only, network-disabled Linux containers with synthetic data and Node 24.17.0. Root dependency fixtures came from main’s manifest rather than the lockfile; exact-head GitHub CI covers locked full checks where green. No production credentials were supplied.
… token validation
charan-rathore
changed the title
Summary
Reject malformed numeric continuations and magnitude suffixes in price watches without rejecting normal sentence commas or subscription separators. The matcher is extracted as a pure helper with table tests; worker-path checks cover a valid price and malformed values that must not trigger notifications.
Resolve the current-main conflict while retaining upstream's whitespace-normalized
containsvalue and replacing only the price matching path with the reviewed helper. The merge preserves both the existing PR head and actual upstream maina4944f4as parents.Validation
e2834ecb34f6e2a4a80696188f017140240eac9e.$1,0and$9Mworker cases fail while the valid-price control passes. The exact fixed service was restored afterward.No live website navigation, production account, full workspace suite, mobile export or local application build is claimed. The prior partial-run/test-count notes are superseded by the completed targeted run above.
Current CI limitation
On commit
906aa53, six platform/browser/container checks passed. The quality job passed lint/types and reached 536 passing tests, then itstests/model-outcome.test.tsprocess aborted in Node 24.22.1/V8 withCheck failed: jit_page_->allocations_.erase(addr) == 1in the Wasm/JIT native stack. This is not a passing CI result. The unchanged test file's five cases pass locally on native arm64 Node 24.22.2. No unrelated application or runtime workaround was added. The base commit's seven checks are green; a maintainer-controlled retry is still required to determine whether this run's native crash recurs.Failed quality job.